Skip to content

Fix some more stream names to be IDs, and mark the rest #5246

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 15 commits into from
Feb 18, 2022

Conversation

gnprice
Copy link
Member

@gnprice gnprice commented Feb 18, 2022

Our goal in this series isn't yet to resolve the issue #3918, but to
get to where it's easy to audit our code for the remaining things we
need to do to complete it.

The first 10 commits in this PR are focused on that. The remaining
6 commits go on to fix some of the remaining instances of the issue.
There are a handful of others that remain.


There are two kinds of searches one wants to do in such an audit:

  • (a) Searching for known instances of the issue, in order to find
    remaining items to work on and eventually to confirm there are
    none left. For this, we want simple systematic patterns to
    search for.

  • (b) Searching for yet-unknown instances of the issue. This is
    inherently fuzzier; one searches for patterns like /stream.name/
    or /streamName/ or even /stream.*name/i to try to find code that
    refers to stream names, which produce a lot of matches including
    some legitimate uses and some that aren't even about stream names.

    For this, the main thing we can do is make it as simple as
    possible when reading such search results to confirm that a given
    spot is a known instance of the issue, in order to focus on those
    that might be unknown.

For the sake of (a), we add TODO-3918 comments at each instance
that's actionable on its own, and references to #3918 where there's
another TODO comment that the fix blocks on.

For the sake of (b), when a given instance of the issue pops up in a
number of scattered places (like the definition and each call site of
a given function that takes a stream name), we add references to #3918
at each of those places even when we only need one TODO comment.


I believe this covers all remaining instances of #3918. That means
that every place we refer to a stream name should either:

As a result, a search like git grep TODO..3918 will find all the
remaining places we need to act in order to close issue #3918,
according to the plan I described here:
#3918 (comment)

The broader search git grep -w 3918 will additionally find all the
places that are (known) instances of the issue but are blocked purely
on server upgrades.

@chrisbobbe
Copy link
Contributor

Looks like there are some rebase conflicts, would you mind resolving those?

@gnprice
Copy link
Member Author

gnprice commented Feb 18, 2022

Sure -- done.

Copy link
Contributor

@chrisbobbe chrisbobbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks! LGTM; please merge at will. I left a tiny comment about an existing code comment.

@@ -38,6 +38,9 @@ export const fromAPNsImpl = (rawData: ?JSONableDict): Notification | void => {
//
// For the format this parses, see `ApnsPayload` in src/api/notificationTypes.js .
//
// Though what it actually receives is more like this:
// $Rest<ApnsPayload, {| aps: mixed |}>
// See comment below about PushNotificationsIOS.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the existing comment below, about PushNotificationsIOS, slightly wrong?

  // PushNotificationsIOS filters out `aps`, parses it, and hands us the rest
  // as "data". Pretty much any iOS notifications library should do
  // the same, but we don't rely on that.

When it says "as 'data'", it makes me think we're working with an object that has a data property, and I don't think we are.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's a bit confusing. There is that getData method, which is how we get the object that's passed to this function; I think that may be what this comment was meant to refer to.

But also the whole comment that's part of, and the stanza that follows it, seem unnecessary:

  const data: JSONableInputDict = (() => {
    if ('aps' in rawData) {
      // eslint-disable-next-line no-unused-vars
      const { aps, ...rest } = rawData;
      return rest;
    } else {
      return rawData;
    }
  })();

If we switched to some other iOS notifications library that had different behavior, we'd be updating this code for the new set of expectations anyway. And as far as this particular point is concerned… none of the subsequent code notices or cares whether there's an aps property on the object. It takes the zulip property, and does nothing with the rest of the object.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an important fact to know when using these functions,
so it belongs in their jsdoc.
This corresponds to the recent changes that made the actual
content of a Narrow include a stream ID and not a stream name.
This is one place where we've correctly used stream IDs, not names,
from the very beginning: 10ba802 in 2017-01, where we first started
handling these events.
Our goal in this series isn't yet to resolve the issue zulip#3918, but to
get to where it's easy to *audit* our code for the remaining things we
need to do to complete it.

There are two kinds of searches one wants to do in such an audit:

 (a) Searching for *known* instances of the issue, in order to find
     remaining items to work on and eventually to confirm there are
     none left.  For this, we want simple systematic patterns to
     search for.

 (b) Searching for yet-*unknown* instances of the issue.  This is
     inherently fuzzier; one searches for patterns like /stream.name/
     or /streamName/ or even /stream.*name/i to try to find code that
     refers to stream names, which produce a lot of matches including
     some legitimate uses and some that aren't even about stream names.

     For this, the main thing we can do is make it as simple as
     possible when reading such search results to confirm that a given
     spot is a known instance of the issue, in order to focus on those
     that might be unknown.

For the sake of (a), we add TODO-3918 comments at each instance
that's actionable on its own, and references to zulip#3918 where there's
another TODO comment that the fix blocks on.

For the sake of (b), when a given instance of the issue pops up in a
number of scattered places (like the definition and each call site of
a given function that takes a stream name), we add references to zulip#3918
at each of those places even when we only need one TODO comment.

This commit adds such comments for two related instances of the issue,
and the next few commits will add them for others.
This property should be optional, because the code still supports
server versions where it's absent.  (As reflected in the comment
there, and more concretely in FcmMessage.kt where it handles the
same user_id property.)

Then marking it optional raises the question: why doesn't Flow
tell us about an error?  In fact we were already not assuming the
property is present: extract.js explicitly handles the case where
it's absent.  But also it turns out that even if we were making
that assumption, Flow wouldn't be able to make the connection,
because we don't use these types in code.  That's different from
how we use most of our API types -- so document it explicitly
with some comments, to help avoid surprises in the future.
The repeated mentions of the issue may look redundant.  But this
way, when the TODO-3918 comments are deleted in the course of
taking care of them, the other references will naturally remain.

That will help with grepping for remaining places we're using
stream names, even while those places aren't directly actionable
because they're blocked on a minimum-server-version upgrade.
This makes it easy to, say, flip `name` from read-only to write-only
in order to audit the remaining uses of stream names.
Or in some cases, there's an existing TODO-server comment, and we
just add a reference to the issue.  (These are places where there's
nothing to be done about the issue before simply dropping support
for older servers, so the proper thing for the TODO itself to refer
to is the server version.)

I believe this covers all remaining instances of zulip#3918.  That means
that every place we refer to a stream name should either:
 * be a legitimate use of stream names;
 * have a TODO-3918 comment; or
 * have a different kind of TODO comment (as it happens, always a
   TODO-server comment), and a mention of zulip#3918.

As a result, a search like `git grep TODO..3918` will find all the
remaining places we need to act in order to close issue zulip#3918,
according to the plan I described here:
  zulip#3918 (comment)

The broader search `git grep -w 3918` will additionally find all the
places that are (known) instances of the issue but are blocked purely
on server upgrades.
Our types say this is a string, so the `|| ''` can't have any
effect.

Just to be sure there wasn't a latent bug somewhere that this
was hiding, I went and traced where this data comes from.
That `streamName` property comes from these lines in
getUnreadStreamsAndTopics, in unreadSelectors:

      const { name, color, in_home_view, invite_only, pin_to_top } =
        subscriptionsById.get(streamId) || NULL_SUBSCRIPTION;

      // …
        streamName: name,

So it's the `name` property either of an actual subscription,
or of NULL_SUBSCRIPTION.

In an actual subscription that's a string, and for that matter
shouldn't be empty.  In NULL_SUBSCRIPTION it's… the empty string.
So effectively we're already doing this fallback up in
getUnreadStreamsAndTopics, where we construct this data in the
first place.

It's not clear that that's the right answer either, but let's at
least do the fallback just once.  We're already doing it
systematically there (for this and several other properties) and
haphazardly in this file (which already has another reference to
streamName without this fallback), so let it be there for now.
This was only used for invoking the callback, which no longer takes it.
This is NFC except in the case where the message's stream ID and
stream name don't match; in that case it fixes a bug.
This way, the use sites are now in the same situation as they would
be if passed a whole Stream object.  That makes it entirely their
responsibility whether they use the stream's name, either
appropriately or inappropriately.

As it happens, there's one callback that uses it appropriately (to
generate a Zulip-flavored-Markdown stream reference in autocomplete)
and one that uses it inappropriately.  The latter is because that's
what some API methods take, and already has appropriate comments.
@gnprice gnprice merged commit 3372bbc into zulip:main Feb 18, 2022
@gnprice
Copy link
Member Author

gnprice commented Feb 18, 2022

Thanks for the review! Merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants